Skip to content

add heap hint support for a few of the x509 functions#7136

Merged
dgarske merged 4 commits intowolfSSL:masterfrom
jpbland1:x509-new-ex
Jan 19, 2024
Merged

add heap hint support for a few of the x509 functions#7136
dgarske merged 4 commits intowolfSSL:masterfrom
jpbland1:x509-new-ex

Conversation

@jpbland1
Copy link
Copy Markdown
Contributor

@jpbland1 jpbland1 commented Jan 17, 2024

Description

Adds heap hint support only for the functions the customer asked for

Fixes zd# 17258

Testing

Uses api and crl tests

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@jpbland1 jpbland1 marked this pull request as ready for review January 17, 2024 16:28
dgarske
dgarske requested changes Jan 17, 2024
View reviewed changes
Comment thread src/x509_str.c Outdated

/* tmp ctx for setting our cert manager */
ctx = wolfSSL_CTX_new(cm_pick_method());
ctx = wolfSSL_CTX_new(cm_pick_method(NULL));
Copy link
Copy Markdown
Member

@dgarske dgarske Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use str->cm->heap

Copy link
Copy Markdown
Contributor Author

@jpbland1 jpbland1 Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, str->cm is checkedfor NULL

Comment thread wolfssl/ssl.h Outdated
WOLFSSL_API WOLFSSL_X509* wolfSSL_X509_new(void);
WOLFSSL_API WOLFSSL_X509* wolfSSL_X509_new_ex(void* heap);
WOLFSSL_API WOLFSSL_X509* wolfSSL_X509_dup(WOLFSSL_X509* x);
WOLFSSL_API WOLFSSL_X509* wolfSSL_X509_dup_ex(WOLFSSL_X509* x, void* heap);
Copy link
Copy Markdown
Member

@dgarske dgarske Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need wolfSSL_X509_dup_ex? Can't the wolfSSL_X509_dup just use x->heap from the previous one? If you have places where heap isn't populated then switch those to using wolfSSL_X509_new_ex.

Copy link
Copy Markdown
Contributor Author

@jpbland1 jpbland1 Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, I had this in my other PR but was following what the customer gave me for this one

@jpbland1
Copy link
Copy Markdown
Contributor Author

jpbland1 commented Jan 17, 2024

retest this please

@jpbland1
Copy link
Copy Markdown
Contributor Author

jpbland1 commented Jan 17, 2024

I'm getting:

Got -132 but expected -171 at line 4947 in ecdsa_invalid_data_tests
Encountered an unexpected result
 ECDSA-INVALID ... FAILED

which I don' think I've changed, lets try one more time:
retest this please

@dgarske
Copy link
Copy Markdown
Member

dgarske commented Jan 17, 2024

@jpbland1 . This is a known issue. @lealem47 is working on it.

@jpbland1 jpbland1 requested a review from dgarske January 17, 2024 20:03
@dgarske dgarske assigned wolfSSL-Bot and unassigned dgarske Jan 17, 2024
dgarske
dgarske approved these changes Jan 17, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@dgarske dgarske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks

@lealem47
Copy link
Copy Markdown
Contributor

lealem47 commented Jan 17, 2024

Jenkins Retest this please

dgarske
dgarske requested changes Jan 17, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@dgarske dgarske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test case for wolfSSL_X509_d2i_ex or adapt an existing one. How have you confirmed all X509 nodes have the heap hint? Did you try with WOLFSSL_HEAP_TEST?

@dgarske dgarske assigned jpbland1 and unassigned jpbland1 and wolfSSL-Bot Jan 17, 2024
@jpbland1
update uses of wolfSSL_X509_new and wolfSSL_X509_d2i
41ea110 
where heap doesn't require a new ex function or struct field to avoid size increase
@jpbland1
Copy link
Copy Markdown
Contributor Author

jpbland1 commented Jan 18, 2024

Please add a test case for wolfSSL_X509_d2i_ex or adapt an existing one. How have you confirmed all X509 nodes have the heap hint? Did you try with WOLFSSL_HEAP_TEST?

Updated the test cases but the question of how to make sure all X509 nodes have the heap hint is why I didn't want to do a half measure on this but we can't blow up the x509small size. I've updated the functions that already have the heap hint and wouldn't require their own ex function and I guess if a customer needs those functions too we can add them but I think we need a long term solution so x509small doesn't break.

Tested working with WOLFSSL_HEAP_TEST.

Comparing the sizes, configured with ./configure --enable-opensslextra=x509small
On 089468f:

$ size -G /usr/local/lib/libwolfssl.so.42.0.0 
      text       data        bss      total filename
    445915     198363      24200     668478 /usr/local/lib/libwolfssl.so.42.0.0

On current commit:

$ size -G /usr/local/lib/libwolfssl.so.42.0.0 
      text       data        bss      total filename
    445979     198535      24200     668714 /usr/local/lib/libwolfssl.so.42.0.0

A difference of 236 bytes

@jpbland1 jpbland1 requested a review from dgarske January 18, 2024 00:09
dgarske
dgarske previously approved these changes Jan 18, 2024
View reviewed changes
@dgarske dgarske assigned wolfSSL-Bot and unassigned dgarske and jpbland1 Jan 18, 2024
JacobBarthelmeh
JacobBarthelmeh requested changes Jan 18, 2024
View reviewed changes
Comment thread src/ssl_certman.c Outdated
Comment thread src/x509_str.c Outdated
@dgarske dgarske merged commit a3a7012 into wolfSSL:master Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dgarske dgarske dgarske approved these changes

@JacobBarthelmeh JacobBarthelmeh JacobBarthelmeh approved these changes

Assignees

@wolfSSL-Bot wolfSSL-Bot

@jpbland1 jpbland1

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jpbland1 @dgarske @lealem47 @JacobBarthelmeh @wolfSSL-Bot